fs: fix fd leak in ReadStream.destroy() #56

jonathanong · 2014-12-04T03:53:40Z

this is a file descriptor leak PR that was opened in joyent/node for a while but had no traction. please pull this or give us directions on the tests!

nodejs/node-v0.x-archive#8178

/cc @rlidwka @dougwilson

dougwilson · 2014-12-04T04:07:04Z

yay!

rvagg · 2014-12-04T04:09:19Z

existing tests all pass with this change? and do you know the reasoning behind the addition here (i.e. have you done a blame here or on readable-stream to figure that out?)

dougwilson · 2014-12-04T04:17:48Z

@rvagg all existing tests passed, yes. The change was part of this commit: nodejs/node-v0.x-archive@44b308b There is no indication for the purpose except that it's a regression that causes code that calls destory() on a ReadStream to leak the fd if the fd didn't happen to open by the time destroy() was called. 0.8 didn't have this behavior since it just called close(), which already does the appropriate checks and even waits for the fd to become available so it doesn't leak.

The original issue was acknowledged as a bug, but it was told there needed to be a test, but how to do that cross platform was never answered, since Node.js does not give access to the list of all open file descriptors.

rvagg · 2014-12-04T05:39:08Z

Here's your test case, pull it in to this PR: rvagg@3d89deb

rvagg · 2014-12-04T07:26:23Z

removed a setImmediate() that was slightly obscuring the problem, use rvagg@69b7102 instead

jonathanong · 2014-12-04T08:24:31Z

@rvagg i love you

rvagg · 2014-12-04T09:12:58Z

lgtm, obviously, need a review from someone else and then I'll deal with the multi-commits appropriately

indutny · 2014-12-04T13:45:25Z

Also, the test and patch should come in one commit.

indutny · 2014-12-04T14:15:47Z

I'm not sure what exactly we are fixing with this, btw. The destroy method appears to be undocumented, and from what I do see from source of lib/fs.js - it is used in a right way at the moment.

.destroy() method is sort of a raw thing and people should know the right way to call it. .close() is more like a public thing for a stream as it handles various internal states and makes them invisible to users.

Additional argument: .destroy() on net.Socket behaves in a same way as .destroy() on ReadableStream at the moment.

I'm generally -1 for this patch, unless a good argument will appear here. +1 for fixing the documentation of fs module and making a note about destroy behaviour and open event.

dougwilson · 2014-12-04T16:01:40Z

@indutny here is the bug:

I create a stream or get it from somewhere.
I pipe that stream into a WritableStream that I created fs.createWriteStream. WriteStream.prototype.destroy is set to ReadStream.prototype.destroy (https://github.com/joyent/node/blob/v0.10.24/lib/fs.js#L1689)
Now WriteStream is busy trying to get a fd from the OS
The stream I piped into the WriteStream closed because of an error before the fd was acquired.
Node.js calls WriteStream.prototype.destory at https://github.com/joyent/node/blob/v0.10.24/lib/stream.js#L87 ; I'm not the one calling destroy. Now the WriteStream leaked a fd.

TL;DR the pipe implementation from Node.js is what is calling .destory on the stream and not .close and it can do it at the wrong time since it doesn't know any better.

indutny · 2014-12-04T16:16:50Z

@dougwilson v0.12 streams are not calling destroy, AFAIK. And your fix is targeting v0.12 branch.

jonathanong · 2014-12-04T18:04:01Z

@rlidwka @rvagg did one of you guys want to take over this PR (close this, open your own branch)? otherwise i'll look into this this weekend

dougwilson · 2014-12-04T18:09:31Z

v0.12 streams are not calling destroy, AFAIK. And your fix is targeting v0.12 branch.

Clearly, it is: https://github.com/iojs/io.js/blob/v0.12/lib/stream.js#L89

indutny · 2014-12-04T18:19:58Z

@dougwilson am I right that this is the legacy streams API, and is not used by fs.WriteStream?

indutny · 2014-12-04T18:20:21Z

Actually v0.10 streams are not calling destroy either, but I didn't check it that much.

dougwilson · 2014-12-04T19:00:35Z

I'm not sure what you're asking, I'm sorry. This is what I'm doing:

var ws = fs.createWriteStream(...)
stream.pipe(ws)

Now stream emits close before the ws has acquired the fd and now ws will never close the file descriptor, since pipe from Node.js core called ws.destroy (yes, in 0.10 and 0.12). You need to look in the pipe implementation, which is in the lib/stream.js file (exact lines linked in various comments above).

indutny · 2014-12-04T19:18:50Z

@dougwilson why that stream is using legacy pipe function? The new one lives there: https://github.com/iojs/io.js/blob/v0.12/lib/_stream_readable.js#L468 and is not using .destroy(). AFAIK, destroy is not a part of Streams API, and should not really be called by anyone.

rlidwka · 2014-12-04T19:20:10Z

@rvagg 's test could be simplified to this:

var fs = require('fs');
var assert = require('assert');
var fdClosed = false;

fs.close = (function(close) {
  return function() {
    fdClosed = true;
    close.apply(fs, arguments);
  }
})(fs.close);

fs.createReadStream(__filename).destroy();

process.on('exit', function() {
  assert(fdClosed);
});

It will fail without and succeed with this patch. But it's implementation-dependent, that's what worried me. I mean, it tests that fs.close is called, but doesn't actually test the leak itself (even though right now it happens to be the same thing).

dougwilson · 2014-12-04T19:26:50Z

@indutny it's what I'm being given by old 0.8-style code from a vendor, as they are inheriting from the old Stream class.

Stream.prototype.pipe exists in the Node.js core codebase. So it sounds like Node.js core is not compatible with itself and even if I get a stream that gets it's pipe method from within the Node.js core code, you're saying I'm not allowed to turn around and use it with fs.createWriteStream? That sounds very odd to me. Is there a way I can fs.createWriteStream and get something that is compatible with the old stream implementation that is still sitting in the Node.js core code, then?

indutny · 2014-12-04T19:26:51Z

@rlidwka see the thing I am talking about is that .destroy() won't be called in case of correct Stream API usage. In fact, this method is undocumented, and there is no reason for exposing it, since we have .close().

rlidwka · 2014-12-04T19:26:52Z

AFAIK, destroy is not a part of Streams API, and should not really be called by anyone.

In this case it should probably be renamed to _destroy.

But what the accepted method to close a stream is? As far as I know, close only exists for fs streams.

indutny · 2014-12-04T19:27:15Z

@rlidwka yes, it is .close().

indutny · 2014-12-04T19:27:58Z

@dougwilson I don't see any reason to keep Stream.prototype.pipe, let's remove it. cc @bnoordhuis @trevnorris @rvagg

indutny · 2014-12-04T19:28:41Z

@rlidwka not sure about removing it either. What are your thoughts on this @bnoordhuis ?

dougwilson · 2014-12-04T19:31:20Z

I don't see any reason to keep Stream.prototype.pipe, let's remove it

This is 100% fine with me for 0.12. Can we do anything for 0.10, perhaps, pretty please?

indutny · 2014-12-04T19:33:45Z

@dougwilson yeah, I guess I'll land your patch :) Could you transform the test to make it explicitly use old streams API. It wasn't really evident what kind of problem you had from the test case that you are submitting ;)

* [Refactoring] change taint to _taint on buffer * [Refactor] Added _taint in unit tests * [Feature] Added method for buffer * [Test] Buffer.new() test for each encoding * [Test] Buffer.from() tests * [Test] Buffer.new() tests * [Test] Buffer.new() tests for one string + each encoding * [Test] Buffer.new() tests for concatenated ascii and utf8 encoding * [Test] Buffer.new() test enhanced for utf8 encoding * [Test] Buffer.new() tests for hex (ascii) encoding * [Test] Buffer.new() removed bug in unit tests * [Test] Buffer.new() tests for hex encoding with utf8 character * [Test] Buffer.new() unit tests for base64 encoding * [Test] Buffer.new() unit tests for base64 unicode encoding * [Test] Buffer.new() refactoring for utf8 + ascii encoding * [Buffer] Reactor taint propagation toString and slice * [Buffer] Remove previously inserted newlines * [Buffer] Taint propagation for Buffer.prototype.toString('hex') * [Test] Buffer.new() unit tests for utf8 encoding * [Feature] Buffer.alloc() keep taint * [Feature] Buffer.alloc() keep (sub-)taint for hex encoding * [Test] Buffer.fill() unit tests * [Feature] Buffer.write() ascii/uft8 taint propagation * [Test] Buffer.concat() unit tests * Buffer.write() refactored * Â[Feature] apply Taint to Partial Buffer * [Feature] Keep taint outsite taint ranges for write/fill * [Fix] Buffer.concat() taint propagation * [Merge] buffer refactoring * [Refactoring] Final buffer status * [Feature] Final status of buffer * [Feature] Final refactored buffer api status

Original commit message: Merged: [deoptimizer] Stricter checks during deoptimization Revision: 506e893b812e03dbebe34b11d8aa9d4eb6869d89 BUG=chromium:1161357 NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=mythria@chromium.org (cherry picked from commit 44d052c19df0801fafdf2be54c899db65e79c67a) Change-Id: I97b69ae11d85bc0acd4a0c7bd28e1b692433de80 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2616219 Reviewed-by: Mythri Alle <mythria@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.8@{nodejs#23} Cr-Original-Branched-From: 2dbcdc105b963ee2501c82139eef7e0603977ff0-refs/heads/8.8.278@{#1} Cr-Original-Branched-From: 366d30c99049b3f1c673f8a93deb9f879d0fa9f0-refs/heads/master@{#71094} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649571 Reviewed-by: Victor-Gabriel Savu <vsavu@google.com> Commit-Queue: Achuith Bhandarkar <achuith@chromium.org> Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#56} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@ad2c5da

Original commit message: Merged: [deoptimizer] Stricter checks during deoptimization Revision: 506e893b812e03dbebe34b11d8aa9d4eb6869d89 BUG=chromium:1161357 NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=mythria@chromium.org (cherry picked from commit 44d052c19df0801fafdf2be54c899db65e79c67a) Change-Id: I97b69ae11d85bc0acd4a0c7bd28e1b692433de80 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2616219 Reviewed-by: Mythri Alle <mythria@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.8@{#23} Cr-Original-Branched-From: 2dbcdc105b963ee2501c82139eef7e0603977ff0-refs/heads/8.8.278@{#1} Cr-Original-Branched-From: 366d30c99049b3f1c673f8a93deb9f879d0fa9f0-refs/heads/master@{#71094} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649571 Reviewed-by: Victor-Gabriel Savu <vsavu@google.com> Commit-Queue: Achuith Bhandarkar <achuith@chromium.org> Cr-Commit-Position: refs/branch-heads/8.6@{#56} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@ad2c5da PR-URL: #38275 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

rvagg force-pushed the v0.12 branch 4 times, most recently from d7e65ff to 185d11c Compare December 4, 2014 10:21

SergeyFromHell mentioned this pull request Jul 11, 2019

Promise .finally gives UnhandledPromiseRejection even if .catch has been used. #28636

Closed

joyeecheung mentioned this pull request Feb 20, 2020

repl: printing big strings results in OOM #25478

Closed

wmertens mentioned this pull request Jun 19, 2020

uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed. #33966

Closed

selimrecep mentioned this pull request Jul 14, 2020

illegal access error with Typescript compiled code #34361

Closed

silverwind mentioned this pull request Aug 20, 2020

https/http2: random crashes on macOS with simple server #34854

Closed

RossHathaway mentioned this pull request Jan 5, 2021

eventLoopUtilization not available in perf_hooks.performance #36801

Closed

carlitorweb mentioned this pull request May 5, 2021

Error: EACCES: permission denied, scandir '/etc/cups/ssl' #38544

Closed

solebox mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities solebox/node#29

Open

enterstudio mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities enterstudio/node#28

Open

UbuntuEvangelist mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities UbuntuEvangelist/node#22

Open

s4sc mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities s4sc/node#28

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix fd leak in ReadStream.destroy() #56

fs: fix fd leak in ReadStream.destroy() #56

jonathanong commented Dec 4, 2014

dougwilson commented Dec 4, 2014

rvagg commented Dec 4, 2014

dougwilson commented Dec 4, 2014

rvagg commented Dec 4, 2014

rvagg commented Dec 4, 2014

jonathanong commented Dec 4, 2014

rvagg commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

jonathanong commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

rlidwka commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

rlidwka commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

fs: fix fd leak in ReadStream.destroy() #56

fs: fix fd leak in ReadStream.destroy() #56

Conversation

jonathanong commented Dec 4, 2014

dougwilson commented Dec 4, 2014

rvagg commented Dec 4, 2014

dougwilson commented Dec 4, 2014

rvagg commented Dec 4, 2014

rvagg commented Dec 4, 2014

jonathanong commented Dec 4, 2014

rvagg commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

jonathanong commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

rlidwka commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014

rlidwka commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

indutny commented Dec 4, 2014

dougwilson commented Dec 4, 2014

indutny commented Dec 4, 2014